fix(catalog): preserve virtual result rows#172
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Virtual row DOM reuse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeployedLanding: https://pr-172.preview.caplets.dev Built from commit 95455aa |
|
| Filename | Overview |
|---|---|
| apps/catalog/src/scripts/virtual-results.ts | Introduces keyed DOM-diffing via renderedRows Map and cursor-based insertBefore loop. Core scroll-reuse logic is sound for rows with IDs, but the index-string fallback key (String(item.key)) means a filter change can map a new row to a cached DOM element that contains a different row's HTML content — updateRowPosition only patches position attributes, not innerHTML. |
| apps/catalog/test/virtual-results.test.ts | Adds a node-reuse test that correctly validates the new DOM-preservation behaviour. The test only scrolls one row-height (72 px), staying within the overscan window, which is appropriate. No coverage for the filter-then-verify-content path on ID-less rows. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[scroll / filter event] --> B[renderVirtualRows]
B --> C[virtualizer.getVirtualItems]
C --> D{for each virtual item}
D --> E[virtualRowKey: row.id ?? String item.key]
E --> F{renderedRows.has key?}
F -- No --> G[renderRow: createElement + innerHTML]
G --> H[renderedRows.set key, element]
F -- Yes --> I[updateRowPosition: transform + aria attrs]
H --> J{element !== cursor?}
I --> J
J -- Yes --> K[insertBefore element, cursor]
J -- No --> L[advance cursor]
K --> L
L --> D
D -- done --> M[cleanup pass: querySelectorAll data-result-row]
M --> N{key in nextKeys?}
N -- Yes --> O[keep element in DOM + map]
N -- No --> P[element.remove + renderedRows.delete key]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[scroll / filter event] --> B[renderVirtualRows]
B --> C[virtualizer.getVirtualItems]
C --> D{for each virtual item}
D --> E[virtualRowKey: row.id ?? String item.key]
E --> F{renderedRows.has key?}
F -- No --> G[renderRow: createElement + innerHTML]
G --> H[renderedRows.set key, element]
F -- Yes --> I[updateRowPosition: transform + aria attrs]
H --> J{element !== cursor?}
I --> J
J -- Yes --> K[insertBefore element, cursor]
J -- No --> L[advance cursor]
K --> L
L --> D
D -- done --> M[cleanup pass: querySelectorAll data-result-row]
M --> N{key in nextKeys?}
N -- Yes --> O[keep element in DOM + map]
N -- No --> P[element.remove + renderedRows.delete key]
Comments Outside Diff (1)
-
apps/catalog/src/scripts/virtual-results.ts, line 236-239 (link)Explicitly clearing
renderedRowson destroy ensures detached DOM elements are released immediately rather than waiting for the closure to be GC'd.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix(catalog): preserve virtual result ro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function virtualRowKey(item: VirtualItem, row: CatalogSearchRow | undefined): string { | ||
| return row?.id ?? String(item.key); | ||
| } |
There was a problem hiding this comment.
virtualRowKey falls back to String(item.key) when row.id is absent. item.key is the virtualizer's output of getItemKey, which is itself visibleRows[index]?.id ?? index — so for an ID-less row the key reduces to String(index). After applySearch replaces visibleRows, index N in the new filtered set may point to a completely different row than index N in the old set. renderedRows.get("N") returns the old element, and because updateRowPosition only patches transform/aria attributes (not innerHTML), the row is displayed with the wrong name, description, and icon. The cleanup pass does not evict it because "N" is still present in nextKeys. If CatalogSearchRow.id is always defined in production data this is dormant, but the explicit fallback in both getItemKey and virtualRowKey signals the type permits absent IDs. Clearing renderedRows (or at minimum evicting non-ID entries) at the start of applySearch before calling renderVirtualRows would close the gap.
1 participant
Summary
Catalog icons should no longer flicker while scrolling the production search results. The virtual list now keeps row DOM nodes alive when they remain in the visible window instead of replacing the entire row group on every scroll update.
Validation
pnpm --filter @caplets/catalog test -- test/virtual-results.test.ts test/search-row.test.tspnpm --filter @caplets/catalog typecheckpnpm --filter @caplets/catalog buildpnpm format:checkpnpm verifySummary by CodeRabbit